destroy request on error to avoid hanging sockets by benzekrimaha · Pull Request #1141 · scality/hdclient

benzekrimaha · 2026-03-16T07:57:56Z

On timeout or other errors the socket was left open; we now destroy the request so the socket is properly closed.
Issue: HD-4352

On timeout or other errors the socket was left open; destroy the request so the socket is properly closed.

bert-e · 2026-03-16T07:57:59Z

Hello benzekrimaha,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Available options

name	description	privileged	authored
`/after_pull_request`	Wait for the given pull request id to be merged before continuing with the current one.
`/bypass_author_approval`	Bypass the pull request author's approval	⭐
`/bypass_build_status`	Bypass the build and test status	⭐
`/bypass_commit_size`	Bypass the check on the size of the changeset `TBA`	⭐
`/bypass_incompatible_branch`	Bypass the check on the source branch prefix	⭐
`/bypass_jira_check`	Bypass the Jira issue check	⭐
`/bypass_peer_approval`	Bypass the pull request peers' approval	⭐
`/bypass_leader_approval`	Bypass the pull request leaders' approval	⭐
`/approve`	Instruct Bert-E that the author has approved the pull request.		✍️
`/create_pull_requests`	Allow the creation of integration pull requests.
`/create_integration_branches`	Allow the creation of integration branches.
`/no_octopus`	Prevent Wall-E from doing any octopus merge and use multiple consecutive merge instead
`/unanimity`	Change review acceptance criteria from `one reviewer at least` to `all reviewers`
`/wait`	Instruct Bert-E not to run until further notice.

Available commands

name	description	privileged
`/help`	Print Bert-E's manual in the pull request.
`/status`	Print Bert-E's current status in the pull request `TBA`
`/clear`	Remove all comments from Bert-E from the history `TBA`
`/retry`	Re-start a fresh build `TBA`
`/build`	Re-start a fresh build `TBA`
`/force_reset`	Delete integration branches & pull requests, and restart merge process from the beginning.
`/reset`	Try to remove integration branches unless there are commits on them which do not appear on the source branch.

Status report is not available.

bert-e · 2026-03-16T07:58:41Z

Request integration branches

Waiting for integration branch creation to be requested by the user.

To request integration branches, please comment on this pull request with the following command:

/create_integration_branches

Alternatively, the /approve and /create_pull_requests commands will automatically
create the integration branches.

Issue: HD-4352

maeldonn

Could you add a test that reproduces the original problem (socket left open on error) and verifies it is now fixed?

SylvainSenechal · 2026-03-20T10:38:06Z

Could you add a test that reproduces the original problem (socket left open on error) and verifies it is now fixed?

Yeah if you are able to add a little test that counts connections or something it's nice, but might be hard I don't know

SylvainSenechal · 2026-03-20T10:49:40Z

lib/src/hdcontroller.js

    }).on('error', (err) => {
        if (!callbackCalled) {
            callbackCalled = true;
+            request.destroy(err);


I did a quick check on this, and I think when we reach the error callback of the request, the socket is already destroyed, so maybe here only need to destroy on line 238 ?

We keep the request.destroy(err)here on purpose:
Node can emit an error after the response callback (e.g., keep‑alive timeout or reset on an already-ended request). In that path the socket is sometimes still in the agent pool; explicitly destroying guarantees the FD is closed and doesn’t leak.
ClientRequest.destroy() is idempotent; if the socket was already torn down, this is a no‑op.
We have regression tests covering the post-response error and stream-error paths to ensure destroy is called and sockets don’t accumulate here: 6d878a0

Stub http.request to emit a synthetic error after response; assert destroy() is invoked with the expected err.code. Issue: HD-4352

maeldonn · 2026-03-24T13:51:49Z

tests/unit/hdSocketCleanup.spec.ts

The tests cover the post-response error cases well, but they all go through GET which never hits the stream branch in _handleRequest. The change from request.end to request.destroy at line 285 only runs when a stream is piped to the request, and nothing in the tests exercises that path.

It would be good to have a test that passes a stream that errors mid-transfer (e.g. through PUT) and checks that request.destroy gets called.

Done here : 6d878a0

maeldonn

If @SylvainSenechal comment is right, we're missing the most important test.

Assert destroy call counts in post-response and stream-error paths to guard against double-destroy regressions. Issue: HD-4352

maeldonn

LGTM. Just two small comments/questions.

maeldonn · 2026-03-30T14:47:11Z

tests/unit/hdSocketCleanup.spec.ts

+                            const afterSockets = countAgentSockets((h as unknown as { httpAgent: unknown }).httpAgent);
+                            try {
+                                assert.ok(
+                                    afterSockets <= baselineSockets + 2,


When the loop runs, the agent can temporarily have one live socket plus one free socket in its pools (keep‑alive reuse or a race between response and the post‑response. The bound is just to ensure we’re not leaking; it tolerates at most one extra active + one free beyond the baseline.

maeldonn · 2026-03-30T14:51:17Z

tests/unit/hdSocketCleanup.spec.ts

        }) as typeof http.request);
    }

+    function stubRequestCaptureDestroy(assignCapture: (cap: DestroyCapture) => void): void {


it seems that there is some duplicated logic between stubRequestCaptureDestroy and stubRequestWithPostResponseError.

Agreed they share most of the setup. I kept two helpers because one injects a post‑response error and the other only captures destroy, but I can fold them into a single helper with an optional post‑response error injection flag to remove the duplication

Unify destroy-capture stubs with optional post-response error injection to reduce duplication and keep tests clearer. Issue: HD-4352

destroy request on error to avoid hanging sockets

2f5aefb

On timeout or other errors the socket was left open; destroy the request so the socket is properly closed.

benzekrimaha changed the base branch from development/1 to development/1.3 March 16, 2026 07:58

scality deleted a comment from bert-e Mar 16, 2026

benzekrimaha marked this pull request as ready for review March 16, 2026 07:59

benzekrimaha requested review from benoit-a, bertranddemiddelaer, ftalbart, lamphamsy, liaud and vietnscality as code owners March 16, 2026 07:59

benzekrimaha requested review from a team, SylvainSenechal and maeldonn March 16, 2026 07:59

bump project version

5e344af

Issue: HD-4352

maeldonn reviewed Mar 19, 2026

View reviewed changes

SylvainSenechal reviewed Mar 20, 2026

View reviewed changes

benzekrimaha force-pushed the improvement/HD-4352-properly-close-sockets branch 2 times, most recently from ce87e45 to 9cc19c9 Compare March 23, 2026 16:16

Add tests for destroy() after post-response ClientRequest errors

7ab390a

Stub http.request to emit a synthetic error after response; assert destroy() is invoked with the expected err.code. Issue: HD-4352

benzekrimaha force-pushed the improvement/HD-4352-properly-close-sockets branch from 9cc19c9 to 7ab390a Compare March 23, 2026 16:16

benzekrimaha requested review from SylvainSenechal and maeldonn March 24, 2026 11:45

maeldonn reviewed Mar 24, 2026

View reviewed changes

benzekrimaha force-pushed the improvement/HD-4352-properly-close-sockets branch from 9502b41 to d80775c Compare March 30, 2026 12:54

benzekrimaha requested a review from maeldonn March 30, 2026 12:54

Tighten socket cleanup regression tests

6d878a0

Assert destroy call counts in post-response and stream-error paths to guard against double-destroy regressions. Issue: HD-4352

benzekrimaha force-pushed the improvement/HD-4352-properly-close-sockets branch from 1df5dd9 to 6d878a0 Compare March 30, 2026 13:09

maeldonn approved these changes Mar 30, 2026

View reviewed changes

Dedup request destroy stubs

d88b1d4

Unify destroy-capture stubs with optional post-response error injection to reduce duplication and keep tests clearer. Issue: HD-4352

SylvainSenechal approved these changes Apr 1, 2026

View reviewed changes

Conversation

benzekrimaha commented Mar 16, 2026 • edited by atlassian bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bert-e commented Mar 16, 2026

Hello benzekrimaha,

Uh oh!

bert-e commented Mar 16, 2026

Request integration branches

Uh oh!

maeldonn left a comment

Choose a reason for hiding this comment

Uh oh!

SylvainSenechal commented Mar 20, 2026

Uh oh!

SylvainSenechal Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

benzekrimaha Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

maeldonn Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benzekrimaha Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

maeldonn left a comment

Choose a reason for hiding this comment

Uh oh!

maeldonn left a comment

Choose a reason for hiding this comment

Uh oh!

maeldonn Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

benzekrimaha Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

maeldonn Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

benzekrimaha Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

benzekrimaha commented Mar 16, 2026 •

edited by atlassian bot

Loading

maeldonn Mar 24, 2026 •

edited

Loading